The calculation size_t src_idx = i + byte_shift is susceptible to integer overflow/wrap-around on 32-bit systems if len is very large (e.g., close to size_t max). If i + byte_shift overflows, src_idx wraps around and can evaluate to a value less than len, leading to incorrect data copying from wrapped-around indices instead of zero-filling. To prevent this, perform the bounds check using subtraction (byte_shift < len - i) before computing src_idx.

        if (byte_shift < len - i)
        {
          size_t src_idx= i + byte_shift;

In mixed-type bitwise operations (e.g., binary_string & numeric), calling val_str on the numeric argument converts it to its decimal string representation (e.g., 255 becomes the 3-byte ASCII string "255" / 0x323535), rather than its binary representation (e.g., 0xFF). This causes mismatched length errors (e.g., VARBINARY(1) & 255 compares length 1 with length 3) or incorrect bitwise operations if the lengths happen to match. To align with standard bitwise behavior (and MySQL 8.0 compatibility), numeric arguments should be converted to a binary string representation of their integer value, padded or truncated to match the length of the other binary string operand.

When the input binary string a is empty (len == 0), calling to->realloc(0) can return a null pointer or do nothing. Subsequently, passing a null pointer to memset (e.g., memset(out_ptr, 0, len)) is technically undefined behavior in C/C++, even if the length is 0. Adding an early exit for len == 0 avoids this potential undefined behavior and improves efficiency by bypassing unnecessary allocation and loop overhead.

    size_t len= a->length();
    if (len == 0)
    { 
      to->length(0);
      to->set_charset(&my_charset_bin);
      item->null_value= false;
      return to;
    }

    if (to->realloc(len))

Here and everywhere: you do not need this line. The expected result is already recorded.

especially this needs to go away

do you really need a table for that?

cut the empty lines please

we'd need to push a warning/error here.
my_error(ER_OUTOFMEMORY, MYF(0), len);

Could be overkill, I can't see a way to call this wthout an arg, but just in case DBUG_ASSERT(arg_count ==2);

normally this would just be null_value= true for the current object. Is null and invalid argument? Do we need to test if (item->arguments[0]->null_value) before val_str?

on style is ok to have a single line (only) in a if like condition without braces if its clear.

my_error(ER_OUTOFMEMORY, MYF(0), len);

There's a large amount of duplication of Func_hanlder_shift_left_bin_to_bin. Can it be refactored leaving just the ::val_str in each shift direction? Include a common argument handler called by val_str.

^ common shared with shift type handers. Can the commonality be in the same class.

Nice initialisation of thd only when needed. Do other null returns need similar warnings?

ER_OUTOFMEMORY

group member of same type together. Keeps the size of the structure smaller due to the way its packed.

Why disable warnings?

This test is written in a similar style to some older tests, which is too complex and convoluted.

There's no need to have multiple mariadb servers to test this. Just one server with multiple remote tables should be sufficient.

Newer single-server tests are typically written like this:

--disable_query_log
--disable_result_log
--source ../../t/test_init.inc
--enable_result_log
--enable_query_log
--source include/have_innodb.inc

set spider_same_server_link= 1;
set global spider_same_server_link= 1;
evalp CREATE SERVER srv FOREIGN DATA WRAPPER mysql
OPTIONS (SOCKET "$MASTER_1_MYSOCK", DATABASE 'test',user 'root');

CREATE TABLE t1 (...) ENGINE=InnoDB;

CREATE TABLE t1_s (...)
ENGINE=SPIDER wrapper=mariadb REMOTE_SERVER=srv REMOTE_TABLE=t1;

# do things to t1_s and check results

drop table t1_s, t1;
drop server srv;
--disable_query_log
--disable_result_log
--source ../../t/test_deinit.inc
--enable_result_log
--enable_query_log

Please update the test accordingly.

See also inline comments along these lines, mainly removal of redundant lines.

Remove

Remove

With a single server, only one block setting query log related variables should be sufficient. Remove this

Remove

Have you checked if it is necessary to disable ps protocol here? Same with ps2 protocols below.

Remove

Remove

remove

The call to thd->alloc<uint32>(m_binary_length * 8) allocates m_binary_length * 8 bytes rather than elements because alloc takes the size in bytes. This leads to a severe buffer overflow when memcpy copies m_binary_length * 8 * sizeof(uint32) bytes into it. It should be changed to allocate the correct size in bytes.

      m_binary_bit_counters= (uint32*) thd->alloc(m_binary_length * 8 * sizeof(uint32));

The call to thd->calloc<uint32>(m_binary_length * 8) allocates m_binary_length * 8 bytes instead of elements. This leads to a severe buffer overflow when writing up to m_binary_length * 8 elements of uint32. It should be changed to allocate the correct size in bytes.

    m_binary_bit_counters= (uint32*) thd->calloc(m_binary_length * 8 * sizeof(uint32));

Using my_strtoll10 to parse unsigned 64-bit integers is incorrect because it clamps values greater than LONGLONG_MAX to LONGLONG_MAX and sets an error. Additionally, my_strtoll10 does not take a length parameter and expects a null-terminated string, which can lead to an out-of-bounds read if tmp_str is not null-terminated. We should use my_strntoull10 with tmp_str.length() instead.

            int conv_error= 0;
            const char *endptr;
            ulonglong bits= my_strntoull10(tmp_str.ptr(),
                                           tmp_str.length(),
                                           &conv_error,
                                           &endptr);

The return value of m_str_value.alloc(m_binary_length) is not checked. If the allocation fails (e.g., due to OOM), m_str_value.ptr() may return nullptr, leading to a null pointer dereference and crash in memset. We should check the return value of alloc() before writing to the buffer. Note that the same issue applies to Item_sum_and::reset_binary_accumulator() and Item_sum_xor::reset_binary_accumulator().

void Item_sum_or::reset_binary_accumulator()
{
  if (!m_str_value.alloc(m_binary_length))
  {
    m_str_value.length(m_binary_length);
    m_str_value.set_charset(&my_charset_bin);
    memset((char*) m_str_value.ptr(), 0x00, m_binary_length);
  }
}

This part is not covered in the spider test, despite this bit added in the spider commit. Can you add test coverage?

This part is not covered in the spider test, despite this bit added in the spider commit. Can you add test coverage?

This part is not covered in the spider test, despite this bit added in the spider commit. Can you add test coverage? Same for other bits added in Item_sum_bit::update_field in the spider commit - the method itself is not reached

Please add test coverage for this branch, same below

This else branch looks very similar to the else branch above - can you refactor to reduce code duplication?